From 6f6d7c8d198b199388f907952cc0c8c80e49f93f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 5 Apr 2016 16:32:13 -0700 Subject: [PATCH] Protect against concurrent access to Cargo.lock CI tests seen to be spurious failing on OSX due to this, I believe it's because one process is concurrently writing out Cargo.lock while the other is trying to read it in, so one of them gets a corrupt version. This would ideally be fixed from `pkg.root()` returning a `Filesystem`, but that was unfortunately such an invasive change that it seemed untenable. Additionally we generally don't write files into the source tree, so if this is the only instance it's perhaps not so bad trying to be robust in to the future. --- src/cargo/ops/cargo_generate_lockfile.rs | 4 +- src/cargo/ops/cargo_pkgid.rs | 4 +- src/cargo/ops/lockfile.rs | 74 ++++++++++++++---------- src/cargo/ops/mod.rs | 3 +- src/cargo/ops/resolve.rs | 2 +- tests/test_cargo_generate_lockfile.rs | 1 + tests/tests.rs | 1 + 7 files changed, 49 insertions(+), 40 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 1b8bb0087..95056f2ce 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -23,7 +23,7 @@ pub fn generate_lockfile(manifest_path: &Path, config: &Config) let resolve = try!(ops::resolve_with_previous(&mut registry, &package, Method::Everything, None, None)); - try!(ops::write_pkg_lockfile(&package, &resolve)); + try!(ops::write_pkg_lockfile(&package, &resolve, config)); Ok(()) } @@ -105,7 +105,7 @@ pub fn update_lockfile(manifest_path: &Path, } } - try!(ops::write_pkg_lockfile(&package, &resolve)); + try!(ops::write_pkg_lockfile(&package, &resolve, opts.config)); return Ok(()); fn fill_with_deps<'a>(resolve: &'a Resolve, dep: &'a PackageId, diff --git a/src/cargo/ops/cargo_pkgid.rs b/src/cargo/ops/cargo_pkgid.rs index 2bf32e627..6c6b1a88c 100644 --- a/src/cargo/ops/cargo_pkgid.rs +++ b/src/cargo/ops/cargo_pkgid.rs @@ -8,9 +8,7 @@ pub fn pkgid(manifest_path: &Path, spec: Option<&str>, config: &Config) -> CargoResult { let package = try!(Package::for_path(manifest_path, config)); - - let lockfile = package.root().join("Cargo.lock"); - let resolve = match try!(ops::load_lockfile(&lockfile, &package, config)) { + let resolve = match try!(ops::load_pkg_lockfile(&package, config)) { Some(resolve) => resolve, None => bail!("a Cargo.lock must exist for this command"), }; diff --git a/src/cargo/ops/lockfile.rs b/src/cargo/ops/lockfile.rs index e95920372..ed0182d0c 100644 --- a/src/cargo/ops/lockfile.rs +++ b/src/cargo/ops/lockfile.rs @@ -1,45 +1,39 @@ -use std::fs::File; use std::io::prelude::*; -use std::path::Path; use rustc_serialize::{Encodable, Decodable}; use toml::{self, Encoder, Value}; use core::{Resolve, resolver, Package}; -use util::{CargoResult, ChainError, human, paths, Config}; +use util::{CargoResult, ChainError, human, Config, Filesystem}; use util::toml as cargo_toml; pub fn load_pkg_lockfile(pkg: &Package, config: &Config) -> CargoResult> { - let lockfile = pkg.root().join("Cargo.lock"); - load_lockfile(&lockfile, pkg, config).chain_error(|| { - human(format!("failed to parse lock file at: {}", lockfile.display())) - }) -} + if !pkg.root().join("Cargo.lock").exists() { + return Ok(None) + } -pub fn load_lockfile(path: &Path, pkg: &Package, config: &Config) - -> CargoResult> { - // If there is no lockfile, return none. - let mut f = match File::open(path) { - Ok(f) => f, - Err(_) => return Ok(None) - }; + let root = Filesystem::new(pkg.root().to_path_buf()); + let mut f = try!(root.open_ro("Cargo.lock", config, "Cargo.lock file")); let mut s = String::new(); - try!(f.read_to_string(&mut s)); - - let table = toml::Value::Table(try!(cargo_toml::parse(&s, path))); - let mut d = toml::Decoder::new(table); - let v: resolver::EncodableResolve = try!(Decodable::decode(&mut d)); - Ok(Some(try!(v.to_resolve(pkg, config)))) -} - -pub fn write_pkg_lockfile(pkg: &Package, resolve: &Resolve) -> CargoResult<()> { - let loc = pkg.root().join("Cargo.lock"); - write_lockfile(&loc, resolve) + try!(f.read_to_string(&mut s).chain_error(|| { + human(format!("failed to read file: {}", f.path().display())) + })); + + (|| { + let table = toml::Value::Table(try!(cargo_toml::parse(&s, f.path()))); + let mut d = toml::Decoder::new(table); + let v: resolver::EncodableResolve = try!(Decodable::decode(&mut d)); + Ok(Some(try!(v.to_resolve(pkg, config)))) + }).chain_error(|| { + human(format!("failed to parse lock file at: {}", f.path().display())) + }) } -pub fn write_lockfile(dst: &Path, resolve: &Resolve) -> CargoResult<()> { +pub fn write_pkg_lockfile(pkg: &Package, + resolve: &Resolve, + config: &Config) -> CargoResult<()> { let mut e = Encoder::new(); resolve.encode(&mut e).unwrap(); @@ -69,20 +63,36 @@ pub fn write_lockfile(dst: &Path, resolve: &Resolve) -> CargoResult<()> { None => {} } + let root = Filesystem::new(pkg.root().to_path_buf()); + // Load the original lockfile if it exists. - if let Ok(orig) = paths::read(dst) { + // + // If the lockfile contents haven't changed so don't rewrite it. This is + // helpful on read-only filesystems. + let orig = root.open_ro("Cargo.lock", config, "Cargo.lock file"); + let orig = orig.and_then(|mut f| { + let mut s = String::new(); + try!(f.read_to_string(&mut s)); + Ok(s) + }); + if let Ok(orig) = orig { if has_crlf_line_endings(&orig) { out = out.replace("\n", "\r\n"); } if out == orig { - // The lockfile contents haven't changed so don't rewrite it. - // This is helpful on read-only filesystems. return Ok(()) } } - try!(paths::write(dst, out.as_bytes())); - Ok(()) + // Ok, if that didn't work just write it out + root.open_rw("Cargo.lock", config, "Cargo.lock file").and_then(|mut f| { + try!(f.file().set_len(0)); + try!(f.write_all(out.as_bytes())); + Ok(()) + }).chain_error(|| { + human(format!("failed to write {}", + pkg.root().join("Cargo.lock").display())) + }) } fn has_crlf_line_endings(s: &str) -> bool { diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs index b9eb78ac3..be772fe8c 100644 --- a/src/cargo/ops/mod.rs +++ b/src/cargo/ops/mod.rs @@ -13,8 +13,7 @@ pub use self::cargo_doc::{doc, DocOptions}; pub use self::cargo_generate_lockfile::{generate_lockfile}; pub use self::cargo_generate_lockfile::{update_lockfile}; pub use self::cargo_generate_lockfile::UpdateOptions; -pub use self::lockfile::{load_lockfile, load_pkg_lockfile}; -pub use self::lockfile::{write_lockfile, write_pkg_lockfile}; +pub use self::lockfile::{load_pkg_lockfile, write_pkg_lockfile}; pub use self::cargo_test::{run_tests, run_benches, TestOptions}; pub use self::cargo_package::package; pub use self::registry::{publish, registry_configuration, RegistryConfig}; diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index f5925a46c..472027d11 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -20,7 +20,7 @@ pub fn resolve_pkg(registry: &mut PackageRegistry, Method::Everything, prev.as_ref(), None)); if package.package_id().source_id().is_path() { - try!(ops::write_pkg_lockfile(package, &resolve)); + try!(ops::write_pkg_lockfile(package, &resolve, config)); } Ok(resolve) } diff --git a/tests/test_cargo_generate_lockfile.rs b/tests/test_cargo_generate_lockfile.rs index c8a9bb6a2..e0231b40c 100644 --- a/tests/test_cargo_generate_lockfile.rs +++ b/tests/test_cargo_generate_lockfile.rs @@ -62,6 +62,7 @@ test!(adding_and_removing_packages { assert!(lock2 != lock3); // remove the dep + println!("lock4"); File::create(&toml).unwrap().write_all(br#" [package] name = "foo" diff --git a/tests/tests.rs b/tests/tests.rs index ecd4ade33..e627cb30c 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -93,6 +93,7 @@ fn process>(t: T) -> cargo::util::ProcessBuilder { p.cwd(&support::paths::root()) .env("HOME", &support::paths::home()) .env_remove("CARGO_HOME") + .env_remove("RUSTC") .env_remove("XDG_CONFIG_HOME") // see #2345 .env("GIT_CONFIG_NOSYSTEM", "1") // keep trying to sandbox ourselves .env_remove("CARGO_TARGET_DIR") // we assume 'target' -- 2.30.2